The attached testcase causes a null-deref crash in mozilla-central rev 8ff550409e1d. ==30981==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fb2917b990a bp 0x7ffe5f66ccf0 sp 0x7ffe5f66c820 T0) #0 0x7fb2917b9909 in AddAnimationForProperty /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:490:30 #1 0x7fb2917b9909 in AddAnimationsForProperty /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:603 #2 0x7fb2917b9909 in nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer(mozilla::layers::Layer*, nsDisplayListBuilder*, nsDisplayItem*, nsIFrame*, nsCSSPropertyID) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:790 #3 0x7fb2917f3e47 in nsDisplayOpacity::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:4754:3 #4 0x7fb291734d38 in mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:4278:32 #5 0x7fb291747585 in mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> const*, unsigned int) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:5532:5 #6 0x7fb2917f7e70 in nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:5139:34 #7 0x7fb2917f891d in nsDisplaySubDocument::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:5190:25 #8 0x7fb291734d38 in mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:4278:32

Caused by bug 1305325.

The animation has no timeline there. This is exactly caused by this commit. https://hg.mozilla.org/integration/autoland/rev/60857c37bcfa Before this commit, we checked that the animation has a valid timeline through PlayState(). PlayState() uses GetCurrentTime(), GetCurrentTime() returns null if the animation has no timeline. We should check the timeline in IsPlaying(). (Or check currentTime?)

I'd like Brian to review this patch, so leaving ni? to him.

Comment on attachment 8830096 [details] Bug 1333539 - Part 1: Do not try to send animations without timeline. https://reviewboard.mozilla.org/r/107012/#review108126 Isn't it still possible for IsMatchForCompositor to return MatchForCompositor::IfNeeded with this change? Should it return MatchForCompositor::No? Or, alternatively, should we allow play-pending animations without a timeline to be run on the compositor as non-playing animations? i.e. just fix the check where we deference GetTimeline() to something like: animation->startTime() = startTime.IsNull() || !aAnimation->GetTimeline() ? TimeStamp() : aAnimation->GetTimeline()-> ToTimeStamp(startTime.Value()); And then keep the change in this patch that makes IsPlaying() return false when there is no timeline?

(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #6) > Comment on attachment 8830096 [details] > Bug 1333539 - Do not try to send animations without timeline. > > https://reviewboard.mozilla.org/r/107012/#review108126 > > Isn't it still possible for IsMatchForCompositor to return > MatchForCompositor::IfNeeded with this change? Should it return > MatchForCompositor::No? > > Or, alternatively, should we allow play-pending animations without a > timeline to be run on the compositor as non-playing animations? i.e. just > fix the check where we deference GetTimeline() to something like: > > animation->startTime() = startTime.IsNull() || !aAnimation->GetTimeline() > ? TimeStamp() > : aAnimation->GetTimeline()-> > ToTimeStamp(startTime.Value()); > > And then keep the change in this patch that makes IsPlaying() return false > when there is no timeline? Ah, good catch! I will try with a test case. I guess we should return MatchForCompositor::No anyway, adding GetTimeline() check will cause null holdTime then.

Thanks! Clearing ni for now since it sounds like you will rework this patch slightly.

I was misunderstanding that animation with null-timeline does not effect any results. Now I am convinced that GetTimeline() check in AddAnimationForProperty() is a proper way. I also notice that we don't normally send animation with null-timeline to the compositor because we check GetTimeline() if the animation is pending state [1]. [1] https://hg.mozilla.org/mozilla-central/file/71224049c0b5/layout/painting/nsDisplayList.cpp#l598 We should change this check to 'GetTimeline() && TracksWallClockTime()', just only for testing mode.

Comment on attachment 8830096 [details] Bug 1333539 - Part 1: Do not try to send animations without timeline. https://reviewboard.mozilla.org/r/107012/#review110174 I don't quite understand the commit message, however. It says, "1333539-2.html is the test case that crashes without Animation->GetTimeline() check in AddAnimationForProperty()." I believe that check is added in part 2. Does that mean this added test crashes without part 2. If so, we should add the test in part 2, right?

Comment on attachment 8831613 [details] Bug 1333539 - Part 2: Send animations with null-timeline to the compositor if necessary. https://reviewboard.mozilla.org/r/108162/#review110176

Comment on attachment 8830096 [details] Bug 1333539 - Part 1: Do not try to send animations without timeline. https://reviewboard.mozilla.org/r/107012/#review110180 ::: layout/painting/nsDisplayList.cpp:488 (Diff revision 2) > } > > const ComputedTiming computedTiming = > aAnimation->GetEffect()->GetComputedTiming(); > Nullable<TimeDuration> startTime = aAnimation->GetCurrentOrPendingStartTime(); > - animation->startTime() = startTime.IsNull() > + animation->startTime() = startTime.IsNull() || !aAnimation->GetTimeline() I meant the GetTimeline() check is here.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/205c3c6c123f Part 1: Do not try to send animations without timeline. r=birtles https://hg.mozilla.org/integration/autoland/rev/a8f1dbc491d8 Part 2: Send animations with null-timeline to the compositor if necessary. r=birtles

https://hg.mozilla.org/mozilla-central/rev/205c3c6c123f https://hg.mozilla.org/mozilla-central/rev/a8f1dbc491d8

Comment on attachment 8830096 [details] Bug 1333539 - Part 1: Do not try to send animations without timeline. Approval Request Comment [Feature/Bug causing the regression]: Bug 1305325 . [User impact if declined]: Crash. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Not yet, just landed on mozilla-central. [Needs manual test from QE? If yes, steps to reproduce]: Yes, please just in case. Open the test case in the comment 0. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: It's low I think. [Why is the change risky/not risky?]: The change squashes the cause of this null deref. [String changes made/needed]: None.

Hello Andrei, could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!

(In reply to Gerry Chang [:gchang] from comment #21) > Hello Andrei, could you help find someone to verify if this issue is fixed > as expected on a latest Nightly build? Thanks! Forwarding this to Brindusa -- her team is taking care of requests on the nightly channel.

Hi Rares, can you help take care of this since Brindusa is out this week?

I've managed to reproduce this issue with Nightly from 2017-01-25 using the test case from comment 0. Verified fixed on latest Nightly 54.0a1 (2017-02-07) across platforms: - Windows 10 x64 - Mac OS X 10.11. - Ubuntu 16.04 x64

Thank you roxana!

Comment on attachment 8830096 [details] Bug 1333539 - Part 1: Do not try to send animations without timeline. Fix a crash and was verified. Aurora53+.

I did a try for aurora branch just in case. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e794ea3d5f5a98fc50905454c39a866f51b93d12

Landed: https://hg.mozilla.org/releases/mozilla-aurora/rev/82c75fd3a2de796351296592c459ab4aa4cd0baf

setting flags